Python: Bound abandoned PolicyEnforcement pending approvals - #7893
Python: Bound abandoned PolicyEnforcement pending approvals#7893Shivani . (Shivani767) wants to merge 2 commits into
Conversation
Prevent unbounded memory growth by capping and expiring unconsumed policy-approval bindings, and clear pending state when an approval is explicitly rejected.
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Bounds abandoned policy-approval state in long-lived Python middleware instances.
Changes:
- Adds FIFO size limits and TTL expiration.
- Adds rejection cleanup logic and regression tests.
- Rejection cleanup is not reached by the normal agent approval flow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
python/packages/core/agent_framework/security.py |
Implements bounded pending-approval lifecycle. |
python/packages/core/tests/test_security.py |
Tests eviction, expiration, and direct rejection handling. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| "approved execution." | ||
| ), | ||
| ) | ||
| elif self._discard_rejected_pending_approval(context): |
There was a problem hiding this comment.
Good catch — rejected approvals never re-enter process().
Updated to notify middleware from _resolve_approval_responses via FunctionMiddlewarePipeline.notify_rejected_approvals → PolicyEnforcementFunctionMiddleware.discard_rejected_policy_approvals, and replaced the direct-process test with a resolver resume regression that asserts pending state is cleared without executing the tool.
Rejected approvals never re-enter function middleware, so notify PolicyEnforcement from _resolve_approval_responses and cover that path with a resume regression test.
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Shivani . (@Shivani767) Nice work on this! I was actually working on #7890 too and had a fix ready locally, but saw your draft first and compared against it instead of duplicating the effort. I verified locally against the original repro — 5,000 unapproved calls are bounded at 256 instead of growing unbounded — and the A few optional thoughts from comparing the approaches, none blocking:
The resolver-side rejection cleanup is a nice catch — I hadn't considered that rejected approvals skip the normal middleware path. To be clear, these are just suggestions, so take or leave them. I won't open a competing PR — I just want to see #7890 fixed correctly, and it doesn't matter whose PR it is. Nice work! |
|
Patel Namraa (@Namraa310806) Thanks a lot, Namraa! Really appreciate you taking the time to verify the fix against the original repro and for sharing these suggestions. I’ll take a closer look at the deterministic clock approach, the optional debug logging for FIFO eviction, and the defense-in-depth check in Glad we can get #7890 fixed without duplicating the work. Thanks again for the review and validation! |
|
Thanks again for the discussion and for the work on #7893! I’ve opened #7996 with a more concise approach to address #7890 while the issue is still unresolved. My intention is not to duplicate the work, but simply to make sure the bug is fixed with a focused approach. Happy to let the maintainers decide which approach, or combination of approaches, is best. |
| pending = self._pending_policy_approvals.get(call_id) | ||
| if pending is None: | ||
| continue | ||
| if self._response_matches_pending(response, call_id, pending.body_signature): | ||
| self._pending_policy_approvals.pop(call_id, None) |
There was a problem hiding this comment.
Could rejection cleanup preserve the session binding enforced for grants? _resolve_approval_responses broadcasts only rejected_responses, and discard_rejected_policy_approvals matches just the call ID and body. If a shared middleware has session B pending under the same call ID and body, session A's rejection clears B's record, so B's valid approval is discarded and requires reapproval. Could we pass invocation_session through the notification and require its session_id to match pending.session_key before removing the entry?
| max_pending_policy_approvals: int = _DEFAULT_MAX_PENDING_POLICY_APPROVALS, | ||
| pending_policy_approval_ttl: timedelta | None = _DEFAULT_PENDING_POLICY_APPROVAL_TTL, |
There was a problem hiding this comment.
Would it make sense to expose these bounds through SecureAgentConfig too? That is the documented setup path and it forwards every other policy option, but callers using it cannot configure max_pending_policy_approvals or pending_policy_approval_ttl without mutating policy_enforcer after construction. Adding matching keyword arguments there would keep all policy configuration in one place.
Motivation & Context
PolicyEnforcementFunctionMiddlewareretains pending policy-approval entries until they are successfully consumed. Whenapproval_on_violation=Trueand approvals are abandoned, rejected, or never returned,_pending_policy_approvalscan grow without bound on a long-lived middleware instance.Description & Review Guide
What are the major changes?
max_pending_policy_approvals, default 256).pending_policy_approval_ttl, default 1 hour;Nonedisables).What is the impact of these changes?
What do you want reviewers to focus on?
_resolve_approval_responsesis the right seam for rejection cleanup.Related Issue
Fixes #7890
Note: Patel Namraa (@Namraa310806) previously offered to work on this; there was no open PR when this was started. Happy to coordinate if preferred.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.